Fix a shutdown race condition in ControlCore #18632
Conversation
microsoft-github-policy-service
bot
added
Issue-Bug
lhecker
force-pushed
the
dev/lhecker/18598-shutdown-crash
branch
from
5302525 to
45d2519
Compare
| _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { this, &DebugTapConnection::_OutputHandler }); | ||
| _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*e*/) { | ||
| StateChanged.raise(*this, nullptr); | ||
| _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { get_weak(), &DebugTapConnection::_OutputHandler }); |
There was a problem hiding this comment.
get_weak() to ensure pending calls during revocation can complete without this being deallocated.
There was a problem hiding this comment.
does this by chance fix the issue where closing a connection while the debug tap is on it crashes terminal
There was a problem hiding this comment.
I wasn't aware we had such an issue. It's quite likely that this fixes it.
| _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) { | ||
| ConnectionStateChanged.raise(*this, nullptr); | ||
| }); | ||
| _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionStateChangedHandler }); |
There was a problem hiding this comment.
get_weak() to ensure pending calls during revocation can complete without this being deallocated. (Same below.)
| // TODO: This manual event revoking doesn't make much sense. | ||
| // We could just drop the old connection. Why have all that Close() stuff? | ||
| // It also shouldn't need to be exposed to the outside. I suspect we can only | ||
| // improve this though, once drag/drop of tabs doesn't use "startup actions" anymore. |
There was a problem hiding this comment.
(For future code archealogists.)
| // First create the render thread. | ||
| // Then stash a local pointer to the render thread so we can initialize it and enable it | ||
| // to paint itself *after* we hand off its ownership to the renderer. | ||
| // We split up construction and initialization of the render thread object this way | ||
| // because the renderer and render thread have circular references to each other. | ||
| auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>(); | ||
| auto* const localPointerToThread = renderThread.get(); |
There was a problem hiding this comment.
This has always been a thorn in my eye, so I fixed it in this PR by moving the RenderThread into the Renderer. No more ownership issues.
|
|
||
| _renderer.reset(); | ||
| _renderEngine.reset(); |
There was a problem hiding this comment.
I removed this, because I fixed the member order of the class.
src/renderer/base/renderer.cpp
Outdated
| { | ||
| AddRenderEngine(rgpEngines[i]); | ||
| } | ||
| THROW_IF_FAILED(_pThread->Initialize(this)); |
There was a problem hiding this comment.
I always forget... is this guaranteed to be usable in the constructor? is Initialize safe for this if this is partially constructed?
There was a problem hiding this comment.
this is a valid pointer in the constructor, it's just that the members aren't necessarily fully initialized yet.
|
I know the tests probably interact with RenderThread in weird ways. |
I found two issues:
winrt::eventdoesn't extend the lifetime of its owning struct, nor does it block during revocation until pending calls have completed (= destruction of a callee can occur while it's being called).ControlCoreis bad in general (because it's unsafe), but theOutputIdlehandling in particular was just wrong.(Hopefully) Closes #18598
Validation Steps Performed